Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(mainnet): pallet_proxy config & tests #441

Merged
merged 9 commits into from
Feb 4, 2025
Merged

refactor(mainnet): pallet_proxy config & tests #441

merged 9 commits into from
Feb 4, 2025

Conversation

al3mart
Copy link
Collaborator

@al3mart al3mart commented Jan 21, 2025 •
edited
Loading

Extends proxy config module for the pallet:

  • pallet_proxy

Notable configuration items are:

  • pallet_proxy

Updated deposits for mainnet runtime:

// One storage item; key size 32 + 8.
pub const ProxyDepositBase: Balance = deposit(1, 40);
// Additional storage item size of AccountId 32 bytes + ProxyType 1 byte + BlockNum 4 bytes.
pub const ProxyDepositFactor: Balance = deposit(0, 37);
// One storage item; key size 32, value size 16.
pub const AnnouncementDepositBase: Balance = deposit(1, 48);
// Additional storage item 32 bytes AccountId + 32 bytes Hash + 4 bytes BlockNum.
pub const AnnouncementDepositFactor: Balance = deposit(0, 68);
type MaxPending = 32;
type MaxProxies = 32;

[sc-2205]

@al3mart al3mart force-pushed the al3mart/refactor-mainnet-config branch 3 times, most recently from 7359e7a to 9594a3b Compare January 24, 2025 17:20
@al3mart al3mart force-pushed the al3mart/refactor-mainnet-proxy branch from c4126d4 to f73d4e5 Compare January 27, 2025 17:25
@al3mart al3mart marked this pull request as ready for review January 27, 2025 18:06
@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.68421% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.46%. Comparing base (9594a3b) to head (f318840).
Report is 2 commits behind head on al3mart/refactor-mainnet-config.

Files with missing lines Patch % Lines
runtime/mainnet/src/config/proxy.rs 93.68% 6 Missing ⚠️ 
@@                         Coverage Diff                         @@
##           al3mart/refactor-mainnet-config     #441      +/-   ##
===================================================================
+ Coverage                            71.02%   71.46%   +0.44%     
===================================================================
  Files                                   73       75       +2     
  Lines                                13748    13971     +223     
  Branches                             13748    13971     +223     
===================================================================
+ Hits                                  9764     9984     +220     
- Misses                                3717     3720       +3     
  Partials                               267      267
Files with missing lines Coverage Δ
runtime/mainnet/src/config/proxy.rs 83.17% <93.68%> (+83.17%) ⬆️

... and 3 files with indirect coverage changes

peterwht
peterwht reviewed Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@peterwht peterwht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only concern is around the ProxyDepositBase and ProxyDepositFactor.

  1. Wasn't able to calculate the value size myself (so not sure where the 16 comes from)
  2. Our deposits are several times more expensive than the polkadot relay.

runtime/mainnet/src/config/proxy.rs Show resolved Hide resolved
runtime/mainnet/src/config/proxy.rs Outdated
@@ -41,6 +39,17 @@ impl InstanceFilter<RuntimeCall> for ProxyType {
}
}

parameter_types! {
// One storage item; key size 32, value size 16.
pub const ProxyDepositBase: Balance = deposit(1, 48);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have 6 times the amount of bytes Polkadot has. Any reason not to just align to the Polkadot relay chain on this: https://github.com/polkadot-fellows/runtimes/blob/main/relay/polkadot/src/lib.rs#L977

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly. Double checking after your comment I'm also seeing that I did the following miscalculation:

the number we have for ProxyDepositBase was the result of adding AccountId + Balance, but I realize that we should be accounting for 8 bytes, which is the correct length of the key of the map.

I also added the bytes for Balance because the deposited amount is always stored. Although, as it is the size corresponding to the deposit itself, I could see why it is not being included on Polkadot's configuration.

ProxyDepositFactor reasoning in the comment above its declaration, what's the concern ?

Copy link
Collaborator Author

@al3mart al3mart Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d4c936e

As discussed off GH this change updates ProxyDepositBase from deposit(1, 48) to deposit(1, 40).

@al3mart al3mart force-pushed the al3mart/refactor-mainnet-proxy branch from 0716b86 to cd7ebb2 Compare January 29, 2025 18:48
peterwht
peterwht approved these changes Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@peterwht peterwht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for resolving the deposit issue. Makes much more sense now.

Daanvdplas
Daanvdplas reviewed Jan 31, 2025
View reviewed changes
Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the explanation why are we not using the values from pop-common anymore. Other than that, looking good!

runtime/mainnet/src/config/proxy.rs Show resolved Hide resolved
runtime/mainnet/src/config/proxy.rs Outdated Show resolved Hide resolved
@al3mart al3mart mentioned this pull request Jan 31, 2025
Draft
11 tasks
@al3mart al3mart requested a review from Daanvdplas February 3, 2025 13:11
Daanvdplas
Daanvdplas approved these changes Feb 3, 2025
View reviewed changes
Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for the updated tests, very good!

runtime/mainnet/src/config/proxy.rs Outdated Show resolved Hide resolved
@al3mart al3mart merged commit b71c3a8 into al3mart/refactor-mainnet-config Feb 4, 2025
15 checks passed
@al3mart al3mart deleted the al3mart/refactor-mainnet-proxy branch February 4, 2025 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Daanvdplas Daanvdplas Daanvdplas approved these changes

@peterwht peterwht peterwht approved these changes

Assignees
No one assigned
Labels
ready-for-final-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@al3mart @codecov-commenter @peterwht @Daanvdplas